-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade addon to eslint 8 #108
Upgrade addon to eslint 8 #108
Conversation
adjust configs to match new feature set
@@ -38,10 +35,10 @@ module.exports = { | |||
<% } %> // node files | |||
{ | |||
files: [ | |||
'./.eslintrc.js', | |||
'./.eslintrc.cjs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about *.cjs
?
and .prettierrc.js
should be cjs, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, I suppose, this is how I made my "One lint config" for all apps, addons, v2 addons: https://github.com/NullVoxPopuli/eslint-configs/blob/main/configs/ember.js#L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
*.cjs
? and.prettierrc.js
should be cjs, too
only the files that I've touched here are delivered from this repo (eg https://github.com/embroider-build/addon-blueprint/blob/main/files/__addonLocation__/.eslintrc.cjs), the rest seem to be from the base addon
as far as I've generated the addon with this command it creates a .prettierrc.js
and not a .cjs file hence I've left that as is
the changes here are correct as for the output of this blueprint (to the best of my ability to verify)
not sure what if anything you expect to be different here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that if we throw type=module in the package.json, the prettier file will be required to be cjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that if we throw type=module in the package.json, the prettier file will be required to be cjs
these are regex's so how about I make all options valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean? All the 'node files' should be .cjs, and then this eslint entry can be *.cjs
instead of the 4 or 5 lines it is now. But, i think bucause all our browser code is in src, we could probably safely assume all *.{cjs,js}
are node. The toplevel cjs in def node, and the top level js would be esm. One eslint entry to rule them all! (Of the package root lol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(tho, because parserType is 'script', only cjs is supported atm -- kinda obnoxious that esm and cjs need different parsers.)
@NullVoxPopuli fyi resolved the conflicts on this pr following the merge of the other dep updates |
most excellent! |
looks like the prettier config may need a tweak? |
@@ -3,7 +3,7 @@ | |||
<% } %> "plugins": [<% if (typescript) { %> | |||
["@babel/plugin-transform-typescript", { "allowDeclareFields": true }],<% } %> | |||
"@embroider/addon-dev/template-colocation-plugin", | |||
["@babel/plugin-proposal-decorators", { "legacy": true }], | |||
["@babel/plugin-proposal-decorators", { "decoratorsBeforeExport": true }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@void-mAlex @NullVoxPopuli When setting decoratorsBeforeExport
to true
, does one need to configure their v2 addon somehow more?
Some tests for the docs and test apps for ember-container-query
failed locally and in CI: https://github.com/ijlee2/ember-container-query/actions/runs/5124925949
# One type of error
Error: Assertion Failed: The options object passed to tracked() may only contain a 'value' or 'initializer' property, not both. Received: [kind,key,placement,descriptor,ini
# Another type
ReferenceError: Cannot access 'ContainerQueryComponent' before initialization
# And another
TypeError: An element descriptor's .kind property must be either "method" or "field", but a decorator created an element descriptor with .kind "undefined"
I couldn't find examples of other addons that currently use decoratorsBeforeExport: true
.
https://emberobserver.com/code-search?codeQuery=decoratorsBeforeExport&fileFilter=babel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this was probably a mistake, we should bring to legacy
- can you PR an update? I have my release machine setup atm, so I can push it out pretty quick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will create a PR, thanks for the quick feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NullVoxPopuli saw this comment from you but unsure what action would be needed
https://github.com/embroider-build/addon-blueprint/blob/main/index.js#L74